Conversation
lcovar
changed the title
OttoAllmendinger
left a comment
OttoAllmendinger
left a comment
There was a problem hiding this comment.
we are on the right track
CONVENTIONS.md
Outdated
| const signedBytes = tx.toBytes(); | ||
|
|
||
| // For parsing — decode instructions | ||
| const parsed = parseTransaction(txBytes, context); |
There was a problem hiding this comment.
actually would prefer if parseTransaction took the tx as an argument here
There was a problem hiding this comment.
parseTransaction takes bytes to keep it decoupled from Transaction, most callers start with bytes from an api anyway
There was a problem hiding this comment.
wouldn't this be a typical flow?
if parseTransaction takes Transaction we only have to decode once
const tx = Transaction.fromBytes(bytes);
const parsed = parseTransaction(tx);
if (validateParsed(parsed, buildParams)) { throw new Error(); }
tx.sign(privkey);
There was a problem hiding this comment.
youse right. udpated, refactor prs for solana/dot coming up after.
CONVENTIONS.md
Outdated
| } | ||
|
|
||
| // For high-level explanation — business logic | ||
| const explained = explainTransaction(txBytes, options); |
There was a problem hiding this comment.
also we probably need to figure out what the distinction between parse and explain is
maybe just remove the explain example here
There was a problem hiding this comment.
agree, removing.
parseTransaction returns a plain data object with decoded transaction data, Transaction.fromBytes is for signing. explain is basically parseTransaction with bitgo business logic on top, derives the type, extracts outputs/inputs, computes fees. its what bitgojs expects for the explain flow
There was a problem hiding this comment.
Maybe it could live outside the wasm packages then and be built around parseTransaction? at least explainTransaction should be in a bitgo-specific subfolder
For wasm-utxo we have a BitGoPsbt class with a parseWithWalletKeys() and both bitgo explainTransaction and parseTransaction are built around that outside the wasm-utxo package.
There was a problem hiding this comment.
Ok, currently for dot/sol i have it here but im convinced, it should live in bitgojs, ill pull it out.
Remove `explainTransaction` from wasm-solana per the convention that explain logic belongs in BitGoJS, not in wasm packages (PR #176). - Delete `explain.ts` — type derivation, output/input extraction, fee calculation all move to BitGoJS sdk-coin-sol - Change `parseTransaction` to accept a `Transaction` object instead of raw bytes, avoiding double deserialization when the caller already has a Transaction from `fromBytes()` - Add `parse_from_transaction` Rust entry point that accepts a pre-deserialized Transaction, with shared logic in `parse_transaction_inner` - Remove all explain-related exports from index.ts (ExplainedTransaction, TransactionType, StakingAuthorizeInfo, etc.) - Fix pre-existing clippy warnings in intent/build.rs The wasm package now only provides: - `Transaction.fromBytes(bytes)` for deserialization/signing - `parseTransaction(tx)` for decoded instruction data - Builder functions for transaction construction BTC-0
Remove `explainTransaction` from wasm-solana per the convention that explain logic belongs in BitGoJS, not in wasm packages (PR #176). - Delete `explain.ts` — type derivation, output/input extraction, fee calculation all move to BitGoJS sdk-coin-sol - Change `parseTransaction` to accept a `Transaction` object instead of raw bytes, avoiding double deserialization when the caller already has a Transaction from `fromBytes()` - Add `parse_from_transaction` Rust entry point that accepts a pre-deserialized Transaction, with shared logic in `parse_transaction_inner` - Remove all explain-related exports from index.ts (ExplainedTransaction, TransactionType, StakingAuthorizeInfo, etc.) - Fix pre-existing clippy warnings in intent/build.rs The wasm package now only provides: - `Transaction.fromBytes(bytes)` for deserialization/signing - `parseTransaction(tx)` for decoded instruction data - Builder functions for transaction construction Ticket: BTC-3091
Document 0x-prefixed hex exception for external system wire formats, context/material at deserialization time, toBroadcastFormat convention, and explain logic boundary between wasm and BitGoJS.
2 participants
documents 7 architectural patterns enforced in code reviews across wasm-utxo, wasm-solana, wasm-mps, and future packages.
these are design decisions that could reasonably be made differently but we've standardized on:
distilled from otto's recurring review comments. prevents review churn by making the patterns explicit.
ref: wasm-dot PR #145 discussion about convention alignment